Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #379, Enum with 0 value not shown on Example #452

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

njqdev
Copy link

@njqdev njqdev commented Jun 1, 2019

See #379

The bug seems to be caused by

if (value is ValueType && value.Equals(value.GetType().GetDefaultValue())) return true;

returned to

where !info.PropertyValue.IsEmpty()

I ran all tests after the change and none of them failed.

@moh-hassan
Copy link
Collaborator

+1
That fix the bug

@njqdev
Copy link
Author

njqdev commented Jun 2, 2019

My change caused another issue so you might want to close this.

see #379 (comment)

@moh-hassan
Copy link
Collaborator

Instead of closing the Pr, you can convert the PR to WIP (Work In Progress) by modifying the title and add WIP as a prefix, and take your time to resolve the side effect.

I think these methods are needed to be added:

  object  GetDefaultValue(this OptionSpecification option )
  {
    //get the default value from the specification of option
  }

bool  HasDefaultValue(this OptionSpecification option )
  {
    //if  the option has a default value from specification return  true otherwise return false
  }

and IsEmpty() can be based on the computation of these methods.

I see IsEmpty() is calculated from the default value of the BaseType and it should also calculated based on its spec Default.

L252

        if (value is ValueType && value.Equals(value.GetType().GetDefaultValue())) return true;

@njqdev
Copy link
Author

njqdev commented Jun 3, 2019

Sure, I'll give it a try this week or the next

@njqdev njqdev changed the title Fix #379, Enum with 0 value not shown on Example WIP Fix #379, Enum with 0 value not shown on Example Jun 3, 2019
@njqdev njqdev changed the title WIP Fix #379, Enum with 0 value not shown on Example [WIP] Fix #379, Enum with 0 value not shown on Example Jun 3, 2019
@njqdev
Copy link
Author

njqdev commented Jun 11, 2019

The best I could do was to change it to:
if (!spec.Required && value is ValueType && value.Equals(value.GetType().GetDefaultValue())) return true;
This solves the issue by always including required value types, and skipping non-required ones which have a default value.

At first I tried replacing the line with this:

if (spec.HasDefaultValue() && value.Equals(spec.DefaultValue.FromJust())) return true;

but it broke some UnParserExtensionsTests.

For example:

UnParsing_instance_returns_command_line

Message: Expected string to be equivalent to 
"-i 1 2 3" with a length of 8, but 
"-i 1 2 3 0" has a length of 10.

The BaseType default was included even though it wasn't supposed to be.

@njqdev njqdev changed the title [WIP] Fix #379, Enum with 0 value not shown on Example Fix #379, Enum with 0 value not shown on Example Jun 11, 2019
@nemars nemars mentioned this pull request Sep 15, 2019
@moh-hassan moh-hassan force-pushed the develop branch 2 times, most recently from b211712 to 746885a Compare February 3, 2020 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants